ARROW-6314: [C++] Implement IPC message format alignment changes, provide backwards compatibility and "legacy" option to emit old message format by wesm · Pull Request #5211 · apache/arrow

wesm · 2019-08-28T02:17:33Z

This also moves the alignment multiple to IpcOptions and adds the IpcOptions argument to more functions.

…ions in more APIs

wesm · 2019-08-28T05:19:29Z

Will fix the CI issues and arrow_cuda failure (don't currently have CUDA installed on my laptop)

emkornfield · 2019-08-28T05:43:12Z

Took a quick scan through and seems reasonable. One question about if we want to memcpy for pre-0.15.0 version.

pitrou

LGTM overall.

pitrou · 2019-08-28T10:29:55Z

I pushed a fix for the CUDA issue.

… case

wesm · 2019-08-28T15:48:46Z

@kszucs not sure what's up with the Buildbot builds

-- Could NOT find LLVM (missing: LLVM_DIR)
CMake Error at cmake_modules/FindLLVM.cmake:34 (find_package):
  Could not find a package configuration file provided by "LLVM" (requested
  version 7.1) with any of the following names:
    LLVMConfig.cmake
    llvm-config.cmake
  Add the installation prefix of "LLVM" to CMAKE_PREFIX_PATH or set
  "LLVM_DIR" to a directory containing one of the above files.  If "LLVM"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  src/gandiva/CMakeLists.txt:31 (find_package)

kszucs · 2019-08-28T16:28:37Z

@ursabot build

wesm · 2019-08-28T16:36:27Z

@emkornfield @tianchen92 note that Java is failing in a really ugly way when sent this new message format

Incompatible files
null
16:27:42.478 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.NullPointerException: null
	at org.apache.arrow.vector.ipc.message.MessageSerializer.deserializeDictionaryBatch(MessageSerializer.java:504)
	at org.apache.arrow.vector.ipc.ArrowFileReader.readDictionaryBatch(ArrowFileReader.java:178)
	at org.apache.arrow.vector.ipc.ArrowFileReader.readDictionary(ArrowFileReader.java:127)
	at org.apache.arrow.vector.ipc.ArrowFileReader.initialize(ArrowFileReader.java:110)
	at org.apache.arrow.vector.ipc.ArrowReader.ensureInitialized(ArrowReader.java:160)
	at org.apache.arrow.vector.ipc.ArrowReader.getVectorSchemaRoot(ArrowReader.java:62)
	at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:191)
	at org.apache.arrow.tools.Integration.run(Integration.java:118)
	at org.apache.arrow.tools.Integration.main(Integration.java:69)
	Suppressed: java.lang.IllegalStateException: Memory was leaked by query. Memory leaked: (1024)
Allocator(ROOT) 0/1024/33428/2147483647 (res/actual/peak/limit)
		at org.apache.arrow.memory.BaseAllocator.close(BaseAllocator.java:454)
		at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:227)
		... 2 common frames omitted

I would guess that there are inadequate checks happening. For example it is only checked whether the metadata length is not equal to 0

https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/ipc/message/MessageSerializer.java#L590

wesm · 2019-08-28T16:54:51Z

Travis CI succeeds except for integration tests, as expected: https://travis-ci.org/wesm/arrow/builds/577931299

Appveyor looking good: https://ci.appveyor.com/project/wesm/arrow/builds/27023793

Going to go ahead and merge this into the integration branch.

…vide backwards compatibility and "legacy" option to emit old message format This also moves the alignment multiple to `IpcOptions` and adds the `IpcOptions` argument to more functions. Closes #5211 from wesm/ARROW-6314 and squashes the following commits: df3b910 <Wes McKinney> Fix MSVC narrowing warning 62758b6 <Wes McKinney> Code review comments. Copy metadata always in prefix_length==4 legacy case 857a571 <Antoine Pitrou> Fix CUDA IPC 71b2fad <Wes McKinney> Add tests exercising backwards compatibility write and read path 4777d2b <Wes McKinney> Implement backwards compatibility and compatibility mode, pass IpcOptions in more APIs 1a38432 <Wes McKinney> Revert changes to submodule 69883cf <Micah Kornfield> verify 8 bytes alignment fixes ubsan for ipc Lead-authored-by: Wes McKinney <wesm+git@apache.org> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: Micah Kornfield <emkornfield@gmail.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>

wesm · 2019-08-28T17:00:37Z

Merged into the integration branch

…vide backwards compatibility and "legacy" option to emit old message format This also moves the alignment multiple to `IpcOptions` and adds the `IpcOptions` argument to more functions. Closes #5211 from wesm/ARROW-6314 and squashes the following commits: df3b910 <Wes McKinney> Fix MSVC narrowing warning 62758b6 <Wes McKinney> Code review comments. Copy metadata always in prefix_length==4 legacy case 857a571 <Antoine Pitrou> Fix CUDA IPC 71b2fad <Wes McKinney> Add tests exercising backwards compatibility write and read path 4777d2b <Wes McKinney> Implement backwards compatibility and compatibility mode, pass IpcOptions in more APIs 1a38432 <Wes McKinney> Revert changes to submodule 69883cf <Micah Kornfield> verify 8 bytes alignment fixes ubsan for ipc Lead-authored-by: Wes McKinney <wesm+git@apache.org> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: Micah Kornfield <emkornfield@gmail.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>

…vide backwards compatibility and "legacy" option to emit old message format This also moves the alignment multiple to `IpcOptions` and adds the `IpcOptions` argument to more functions. Closes apache#5211 from wesm/ARROW-6314 and squashes the following commits: df3b910 <Wes McKinney> Fix MSVC narrowing warning 62758b6 <Wes McKinney> Code review comments. Copy metadata always in prefix_length==4 legacy case 857a571 <Antoine Pitrou> Fix CUDA IPC 71b2fad <Wes McKinney> Add tests exercising backwards compatibility write and read path 4777d2b <Wes McKinney> Implement backwards compatibility and compatibility mode, pass IpcOptions in more APIs 1a38432 <Wes McKinney> Revert changes to submodule 69883cf <Micah Kornfield> verify 8 bytes alignment fixes ubsan for ipc Lead-authored-by: Wes McKinney <wesm+git@apache.org> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: Micah Kornfield <emkornfield@gmail.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>

emkornfield and others added 4 commits August 27, 2019 19:15

verify 8 bytes alignment fixes ubsan for ipc

69883cf

Revert changes to submodule

1a38432

Implement backwards compatibility and compatibility mode, pass IpcOpt…

4777d2b

…ions in more APIs

Add tests exercising backwards compatibility write and read path

71b2fad

wesm requested review from emkornfield and pitrou August 28, 2019 02:17

wesm changed the base branch from master to ARROW-6313-flatbuffer-alignment August 28, 2019 02:19

emkornfield reviewed Aug 28, 2019

View reviewed changes

Comment thread cpp/src/arrow/ipc/message.cc Outdated

emkornfield reviewed Aug 28, 2019

View reviewed changes

Comment thread cpp/src/arrow/ipc/writer.cc Outdated

emkornfield reviewed Aug 28, 2019

View reviewed changes

Comment thread cpp/src/arrow/ipc/writer.cc Outdated

emkornfield reviewed Aug 28, 2019

View reviewed changes

Comment thread cpp/src/arrow/ipc/message.cc Outdated

pitrou reviewed Aug 28, 2019

View reviewed changes

Comment thread cpp/src/arrow/ipc/message.cc Outdated

Comment thread cpp/src/arrow/ipc/message.cc Outdated

Comment thread cpp/src/arrow/ipc/message.cc Outdated

Comment thread cpp/src/arrow/ipc/message.cc Outdated

Fix CUDA IPC

857a571

wesm added 2 commits August 28, 2019 10:37

Code review comments. Copy metadata always in prefix_length==4 legacy…

62758b6

… case

Fix MSVC narrowing warning

df3b910

wesm closed this Aug 28, 2019

wesm deleted the ARROW-6314 branch August 28, 2019 17:00

asfimport mentioned this pull request Sep 12, 2019

[C++] Implement changes to ensure flatbuffer alignment. #22694

Closed

Uh oh!

Conversation

wesm commented Aug 28, 2019

Uh oh!

wesm commented Aug 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield commented Aug 28, 2019

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Aug 28, 2019

Uh oh!

kszucs commented Aug 28, 2019

Uh oh!

wesm commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Aug 28, 2019

Uh oh!

wesm commented Aug 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitrou commented Aug 28, 2019 •

edited

Loading

wesm commented Aug 28, 2019 •

edited

Loading